Remove unnecessary calls to deepcopy(get_type_map) #2121
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2121 +/- ##
==========================================
- Coverage 95.32% 95.19% -0.14%
==========================================
Files 28 28
Lines 2846 2849 +3
Branches 735 736 +1
==========================================
- Hits 2713 2712 -1
- Misses 77 83 +6
+ Partials 56 54 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
Optimizes performance by reducing unnecessary deep copying of type maps when creating NWB container setters. The PR adds a copy parameter to get_type_map() (defaulting to True for backward compatibility) and deprecates the extensions argument in favor of calling load_namespaces() directly on the returned TypeMap.
- Add
copyparameter toget_type_map()to control whether a deep copy is returned - Deprecate the
extensionsargument inget_type_map()with a warning message - Update all test files to use the new pattern of calling
load_namespaces()on the TypeMap
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pynwb/init.py | Adds copy parameter to get_type_map(), deprecates extensions argument, and updates utility functions to use global TypeMap directly |
| src/pynwb/core.py | Updates _get_type_map() to use copy=False for performance optimization |
| tests/unit/test_extension.py | Refactors test calls to use new pattern of get_type_map() followed by load_namespaces() |
| tests/integration/helpers/utils.py | Updates test helper to use new TypeMap loading pattern |
| CHANGELOG.md | Documents the performance fix, API changes, and deprecation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Motivation
Fix #1958. Closes #2075. Similar approach as in #2075. Adds a
copyargument toget_type_mapthat is default True but can be set to False. The False setting is used when creating a setter for each attribute in each NWB container class. Previously, a deepcopy was performed when creating every setter, which significantly hurt performance.This PR deprecates calling get_type_map with any 'extensions' arguments, a use case I have never seen. IMO it's easy for the rare user to just call load_namespaces on the type map than us maintain all this extra convenience logic that even we don't use. See similar PR for hdmf-common in hdmf-dev/hdmf#1302.
It is possible that deepcopying of typemaps in
get_type_mapcan be removed entirely. That will be investigated separately as part of #2120.How to test the behavior?
Checklist
ruff check . && codespellfrom the source directory.